Consolidate Ad Creation #2575
Consolidate Ad Creation #2575
Conversation
github-actions
bot
added
the
changelog: update
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/2460-simplify-paid-ads-setup #2575 +/- ##
=========================================================================
- Coverage 65.0% 62.4% -2.7%
=========================================================================
Files 475 334 -141
Lines 17900 5226 -12674
Branches 0 1269 +1269
=========================================================================
- Hits 11640 3259 -8381
+ Misses 6260 1781 -4479
- Partials 0 186 +186
Flags with carried forward coverage won't be shown. Click here to find out more.
|
asvinb
changed the base branch from
develop
to
feature/2460-simplify-paid-ads-setup
There was a problem hiding this comment.
@asvinb wanted to leave an early review with some observations I'm seeing. I can see that you have copied the onboarding flow's component, SetupPaidAds, to a new AdsCampaign component, which makes sense to me. However, in doing so, it doesn't include the changes made to that form related to #2459. I worry that this is going to lead to some challenging merge conflict resolution. Is it possible to try merging the feature/2459-campaign-creation-flow branch into this branch and ensure those changes are all included?
Additionally, to try to reduce some of the complexity juggling work in parallel with tickets like this one, #2502, and #2503, we should merge all of the PRs targeted to feature/2460-simplify-paid-ads-setup to the feature branch for #2459 and make them one feature rather than two.
| /> | ||
| ) } | ||
|
|
||
| <FaqsSection /> |
There was a problem hiding this comment.
The changes to the AdsCampaign component in #2531 need to be preserved here.
| // because when there is no connected account, it will disable the budget section and set the `amount` to `undefined`. | ||
| const disabledComplete = | ||
| completing === ACTION_SKIP || ! paidAds.isReady || ! isValidForm; | ||
| const shouldShowPaidAdsSetup = ! onboardingSetup || showPaidAdsSetup; |
There was a problem hiding this comment.
The shouldShowPaidAdsSetup logic was removed in #2533. Is there a reason it needs to be added back here?
|
@joemcgill The PR is no longer relevant unfortunately. I'll close it in favour of #2623 |
Changes proposed in this Pull Request:
Closes #2535
Replace this with a good description of your changes & reasoning.
Screenshots:
Detailed test instructions:
Additional details:
Changelog entry